Support oauth2 authentication for pulsar-client-go #313
Conversation
- based on cloud-cli @ bc645b16ca7b7474b132ee1da8b56da35025a616
There was a problem hiding this comment.
Thanks @zymap and @EronWright for contributing this feature! The overall change looks good! A few questions:
- It seems that most of the tests are unit tests. Can we add some integration tests?
- Can you create an issue in https://github.com/apache/pulsar to document this feature in go client page?
pulsar/internal/auth/oauth2.go
Outdated
| func NewAuthenticationOAuth2( | ||
| issuer oauth2.Issuer, | ||
| store store.Store, | ||
| oauth2Type, keyFilePath string) (Provider, error) { |
There was a problem hiding this comment.
I see that you changed this constructor from NewAuthenticationOAuth2(issuer, store) to NewAuthenticationOAuth2(issuer, store, oauth2type, keyfile). Let me explain why I think that's not a good change.
The authentication provider requires an authorization grant to be able to obtain access tokens over time. The grant is a long-lived credential, such as a keyfile or a refresh token. In all cases, the +application+ obtains the grant beforehand and then stores it in a store. The store might be persistent. Later, the application creates an authorization provider with the store as a parameter. The provider loads the grant from the store, and uses it over time to obtain access tokens.
I am trying to explain that the provider is not responsible for obtaining or storing new grants. Hence the signature NewAuthenticationOAuth2(issuer, store).
Now we discuss the special case of initializing the provider from parameters - NewAuthenticationOAuth2WithParams. One could imagine a combination of parameters that would open a pre-existing store but this would require more design work. A simple scenario that is possible now is the keyfile scenario in combination with an in-memory store (where the access token would be discarded when the process terminates). This scenario is implemented in NewAuthenticationOAuth2WithParams.
Would you revert the change to this method that is seen in 97d98f7?
- decouple the issuer parameter from the audience - use issuer information that is in the keyfile
Refactor to support multiple issuers.
go.mod
Outdated
| github.com/sirupsen/logrus v1.4.1 | ||
| github.com/spaolacci/murmur3 v1.1.0 | ||
| github.com/spf13/cobra v0.0.3 | ||
| github.com/spf13/pflag v1.0.3 // indirect | ||
| github.com/stretchr/testify v1.4.0 | ||
| github.com/yahoo/athenz v1.8.55 | ||
| k8s.io/utils v0.0.0-20200619165400-6e3d28b6ed19 |
There was a problem hiding this comment.
This dependency was due to the mock clock right? I recall that you forked it elsewhere, would that work here too?
There was a problem hiding this comment.
The changes LGTM+1, @zymap Can you fix @EronWright comments?
* remove oauth2 module This removes the module definition in the oauth2 subdirectory. Since the oauth2 module is not released independently of the main module, it can/should be treated as a normal module subdirectory. Signed-off-by: Paul Gier <paul.gier@datastax.com> * update tests for newer golang oauth2 dependency Signed-off-by: Paul Gier <paul.gier@datastax.com> * oauth2: add note to readme about possible module error Signed-off-by: Paul Gier <paul.gier@datastax.com> Signed-off-by: Paul Gier <paul.gier@datastax.com> ### Motivation When the oauth2 sub-directory was added in PR #313 there wasn't any justification given (AFAICT) for creating a separate sub-module instead of just treating the oauth2 sources as a normal directory. Since the oauth2 module does not have a separate release cycle, treating it as a sub-module just adds unnecessary complexity. ### Modifications Removed `go.mod` and `go.sum` from the oauth2 sub-directory. Updated the main `go.mod` and `go.sum` to included the necessary dependencies. The main module contained a newer version of `golang.org/x/oauth2` dependency than the oauth2 sub-module, so I had to update some of the tests to match the output of the newer dependency. This type of dependency mismatch will be avoided in the future using a single module defined in the root directory.
Motivation
Support oauth2 authentication for pulsar-client-go